DI: capture expressions#5845
Conversation
Typing analysisNote: Ignored files are excluded from the next sections.
|
🎉 All green!🧪 All tests passed 🎯 Code Coverage (details) 🔗 Commit SHA: 60e8c77 | Docs | Datadog PR Page | Give us feedback! |
BenchmarksBenchmark execution time: 2026-07-14 18:04:19 Comparing candidate commit 60e8c77 in PR branch Found 0 performance improvements and 0 performance regressions! Performance is the same for 48 metrics, 1 unstable metrics.
|
|
@codex review |
There was a problem hiding this comment.
Pull request overview
Adds Dynamic Instrumentation “capture expressions” support for Ruby log probes, enabling selective value capture (via DSL expressions) into snapshot payloads under captureExpressions, with configurable per-fire serialization time budgeting.
Changes:
- Parse
captureExpressions(including per-expression capture limits) from remote config and attach them toDatadog::DI::Probe. - Evaluate capture expressions at probe fire time with a configurable time budget and merge per-expression evaluation errors into snapshot
evaluationErrors. - Extend serializer plumbing to allow per-evaluation overrides for string length / collection size limits; add docs, RBS signatures, and specs.
Reviewed changes
Copilot reviewed 21 out of 21 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| supported-configurations.json | Adds env var metadata for max serialization time budget. |
| spec/datadog/di/probe_spec.rb | Adds unit coverage for probe capture_expressions behavior and rate-limit defaults. |
| spec/datadog/di/probe_builder_spec.rb | Adds remote-config parsing tests for captureExpressions and related behaviors. |
| spec/datadog/di/capture_expression_spec.rb | Adds tests for capture expression / limits value objects and limit resolution behavior. |
| spec/datadog/di/capture_expression_evaluator_spec.rb | Adds tests for evaluation success/failure, timeout behavior, and limit propagation to serializer. |
| sig/datadog/di/serializer.rbs | Updates serialize_value signature to include length and collection_size. |
| sig/datadog/di/probe.rbs | Adds capture-expression-related fields and predicate signature. |
| sig/datadog/di/probe_notification_builder.rbs | Adds capture_expression_evaluator ivar/reader typing. |
| sig/datadog/di/probe_builder.rbs | Adds capture-expression builder helper typings and name pattern constant. |
| sig/datadog/di/capture_expression.rbs | Adds RBS for CaptureExpression and CaptureLimits. |
| sig/datadog/di/capture_expression_evaluator.rbs | Adds RBS for evaluator class and return types. |
| lib/datadog/di/serializer.rb | Threads per-evaluation length/collection_size through recursive serialization. |
| lib/datadog/di/probe.rb | Stores capture-expression config on probes and adjusts default rate limiting. |
| lib/datadog/di/probe_notification_builder.rb | Evaluates capture expressions for snapshots and merges evaluation errors. |
| lib/datadog/di/probe_builder.rb | Parses captureExpressions from remote config and validates names/expr structure. |
| lib/datadog/di/configuration.rb | Introduces max_time_to_serialize_ms DI setting backed by env var. |
| lib/datadog/di/capture_expression.rb | Adds CaptureExpression and CaptureLimits classes plus limit-resolution logic. |
| lib/datadog/di/capture_expression_evaluator.rb | Implements evaluator with time budget, serialization, and error reporting. |
| lib/datadog/di/boot.rb | Ensures capture-expression components are loaded during DI boot. |
| lib/datadog/core/configuration/supported_configurations.rb | Registers new env var in supported configuration list (generated). |
| docs/DynamicInstrumentation.md | Documents capture expressions, behavior choices, and new env var. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5affe59f77
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…d length The CaptureLimits.resolve doc comment promises a per-field fallback chain of expr_limits -> probe -> settings, but for collection_size and length the code went straight from the expression-level override to settings, skipping the probe-level overrides parsed into probe.max_capture_collection_size / probe.max_capture_string_length. That meant a probe-level capture.maxCollectionSize or capture.maxLength from remote config was silently ignored unless repeated on every expression. Adds the probe-level step to both fallback chains so the resolver matches its documented contract. The serializer already accepts length: and collection_size: keyword arguments, so the fix flows end-to-end. Addresses review comments on PR #5845 from @Copilot (3344166066) and @chatgpt-codex-connector (3344167297). Co-Authored-By: Claude <noreply@anthropic.com>
build_capture_expressions called raw.empty? before verifying raw was an Array. A non-Array value from remote config (e.g. an Integer) would raise NoMethodError instead of the intended ArgumentError, and the outer rescue KeyError in build_from_remote_config would not wrap it either — producing a misleading error class even though the bare rescue in Component#parse_probe_spec_and_notify still kept it from escaping. Reorders the early-return: nil short-circuits, then the Array type check, then the empty-array short-circuit. Adds a probe_builder_spec test for the non-Array case. Addresses review comment on PR #5845 from @Copilot (3344166098). Co-Authored-By: Claude <noreply@anthropic.com>
The return-time Context for method probes was built with locals: nil, so capture expressions referencing arg1 or kwarg names resolved as undefined/nil — the condition path at instrumenter.rb:135 already populates locals via serializer.combine_args(args, kwargs, self) but the return-time path didn't. Passes combine_args(args, kwargs, self) as locals when the probe has capture expressions; keeps nil otherwise to avoid an allocation on every method invocation for probes that don't need it. At return time the args reference is the same Ruby object as at entry, so values reflect any in-place mutation by the method body — matching what conditions already see and what cross-tracer convention reports for method exit scope. Adds an integration test exercising a method probe with a capture expression referencing arg1 against a positional argument. Addresses review comment on PR #5845 from @chatgpt-codex-connector (3344167304). Co-Authored-By: Claude <noreply@anthropic.com>
…d length The CaptureLimits.resolve doc comment promises a per-field fallback chain of expr_limits -> probe -> settings, but for collection_size and length the code went straight from the expression-level override to settings, skipping the probe-level overrides parsed into probe.max_capture_collection_size / probe.max_capture_string_length. That meant a probe-level capture.maxCollectionSize or capture.maxLength from remote config was silently ignored unless repeated on every expression. Adds the probe-level step to both fallback chains so the resolver matches its documented contract. The serializer already accepts length: and collection_size: keyword arguments, so the fix flows end-to-end. Addresses review comments on PR #5845 from @Copilot (3344166066) and @chatgpt-codex-connector (3344167297). Co-Authored-By: Claude <noreply@anthropic.com>
build_capture_expressions called raw.empty? before verifying raw was an Array. A non-Array value from remote config (e.g. an Integer) would raise NoMethodError instead of the intended ArgumentError, and the outer rescue KeyError in build_from_remote_config would not wrap it either — producing a misleading error class even though the bare rescue in Component#parse_probe_spec_and_notify still kept it from escaping. Reorders the early-return: nil short-circuits, then the Array type check, then the empty-array short-circuit. Adds a probe_builder_spec test for the non-Array case. Addresses review comment on PR #5845 from @Copilot (3344166098). Co-Authored-By: Claude <noreply@anthropic.com>
The return-time Context for method probes was built with locals: nil, so capture expressions referencing arg1 or kwarg names resolved as undefined/nil — the condition path at instrumenter.rb:135 already populates locals via serializer.combine_args(args, kwargs, self) but the return-time path didn't. Passes combine_args(args, kwargs, self) as locals when the probe has capture expressions; keeps nil otherwise to avoid an allocation on every method invocation for probes that don't need it. At return time the args reference is the same Ruby object as at entry, so values reflect any in-place mutation by the method body — matching what conditions already see and what cross-tracer convention reports for method exit scope. Adds an integration test exercising a method probe with a capture expression referencing arg1 against a positional argument. Addresses review comment on PR #5845 from @chatgpt-codex-connector (3344167304). Co-Authored-By: Claude <noreply@anthropic.com>
8b413eb to
1bf6c26
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7cf491474a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Adds capture-expression support to Ruby log probes.
* Parse the `captureExpressions` field in remote-config LOG_PROBE payloads.
* Evaluate each expression against the probe scope at fire time.
* Emit a `captureExpressions` block in the snapshot under the same
`captures.{lines.<n>|entry|return}` key that full snapshots use.
* Per-fire time budget controlled by
`DD_DYNAMIC_INSTRUMENTATION_CAPTURE_TIMEOUT_MS` (default 200).
Remaining expressions emit a stub `{notCapturedReason: "timeout"}`.
* Snapshot wins at fire time when both `captureSnapshot` and
`captureExpressions` are set on the same probe (matches Python/Java/Go).
* Per-expression evaluation errors are added to `evaluationErrors` with
the failed expression's name; the failed key is omitted from the
`captureExpressions` block.
* Capture-expression probes default to the snapshot rate-limit bucket
(1/sec); existing `sampling.snapshotsPerSecond` overrides this.
Tests cover parsing (including the backend name pattern
`^[a-zA-Z0-9_?]+$`), the rate-limit fallback, per-expression depth
override, time-budget exhaustion, and the success/error/timeout output
shapes.
Known limitation: per-expression `maxLength` and `maxCollectionSize`
are not yet honored end-to-end; the snapshot serializer reads those
two limits from the DI settings directly. Per-expression
`maxReferenceDepth` and `maxFieldCount` work.
- Thread max_length and max_collection_size kwargs through Serializer#serialize_value and its recursive calls, so per-expression CaptureLimits overrides for all four fields work end-to-end (previously only depth and attribute_count were honored). - Add Steep signatures for the new ProbeBuilder helpers, the capture_expression_evaluator accessor on ProbeNotificationBuilder, and the new length/collection_size kwargs on Serializer#serialize_value. - Remove the 'known limitation' note from docs/DynamicInstrumentation.md.
…_SERIALIZE The prior commit added DD_DYNAMIC_INSTRUMENTATION_CAPTURE_TIMEOUT_MS as a new key, which would require central Configuration Registry coordination and broke validate_supported_configurations_v2_local_file. Switch to the existing DD_DYNAMIC_INSTRUMENTATION_MAX_TIME_TO_SERIALIZE env var that .NET already declares (and which is already centrally registered). Its 200 ms default matches our chosen budget; its semantics extend naturally to whole-snapshot serialization if Ruby later applies the budget more broadly. * Rename the setting from capture_expression_timeout_ms to max_time_to_serialize_ms. * Drop the new DD_DYNAMIC_INSTRUMENTATION_CAPTURE_TIMEOUT_MS entry from supported-configurations.json and supported_configurations.rb; add the existing DD_DYNAMIC_INSTRUMENTATION_MAX_TIME_TO_SERIALIZE. * Update docs/DynamicInstrumentation.md to reference the .NET-aligned env var. * Update the capture-expression evaluator and spec to use the new setting name.
…d length The CaptureLimits.resolve doc comment promises a per-field fallback chain of expr_limits -> probe -> settings, but for collection_size and length the code went straight from the expression-level override to settings, skipping the probe-level overrides parsed into probe.max_capture_collection_size / probe.max_capture_string_length. That meant a probe-level capture.maxCollectionSize or capture.maxLength from remote config was silently ignored unless repeated on every expression. Adds the probe-level step to both fallback chains so the resolver matches its documented contract. The serializer already accepts length: and collection_size: keyword arguments, so the fix flows end-to-end. Addresses review comments on PR #5845 from @Copilot (3344166066) and @chatgpt-codex-connector (3344167297). Co-Authored-By: Claude <noreply@anthropic.com>
This PR added two Datadog.logger.debug calls in ProbeBuilder (master had none), reaching for the global logger from module_function code — a Component Pattern violation (component code must use injected dependencies, not globals). Both log paths originate from build_from_remote_config (the second via parse_evaluate_at), and the sole production caller, Component#parse_probe_spec_and_notify, already has the injected DI::Logger facade in scope. - probe_builder.rb: build_from_remote_config now takes a required `logger:` kwarg and threads it into parse_evaluate_at; both Datadog.logger.debug calls become logger.debug - component.rb: pass the component's injected logger at the call site - probe_builder.rbs: update the two signatures - specs: inject a logger double at every build_from_remote_config call site; the two probe_builder_spec assertions now expect on the injected logger rather than the Datadog.logger global Verified: rspec (probe_builder, component, integration instrumentation + notification builder, remote = 137, 0 failures), StandardRB clean, Steep clean.
|
@codex review |
|
Codex Review: Didn't find any major issues. You're on a roll. Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
# Conflicts: # lib/datadog/di/instrumenter.rb # spec/datadog/di/probe_spec.rb
Replace em-dash and arrow characters introduced by this PR's comments with ASCII equivalents (-- and ->).
Replace string-name double("settings", ...) with
instance_double(Datadog::Core::Configuration::Settings, ...) in the
CaptureLimits.resolve and Probe#snapshot_serializer_limits specs so the
settings interface is verified.
The backend permits duplicate captureExpressions names and forwards them unchanged. ProbeBuilder now collapses entries sharing a name, keeping the last occurrence, so each name is evaluated once instead of being evaluated and then overwritten when the snapshot is serialized. The collapse is logged at debug.
Strips all added non-pragma comment lines (374 across lib and spec) from the capture-expressions change. Functional pragmas (frozen_string_literal, standard:disable, steep:ignore) are kept. Code and tests unchanged.
…_TIME_TO_SERIALIZE doc
…n Probe Address review comments (extract conditions as methods on `probe`; `snapshot_serializer_limits` flagged as a delegation candidate): - lib/datadog/di/probe.rb: add `evaluate_at_entry?`, `capture_expressions_only?`, and `capture_entry_expressions?`; `snapshot_serializer_limits` now delegates to `CaptureLimits.resolve`, removing the duplicated probe->settings fallback chain. - lib/datadog/di/instrumenter.rb (535, 613): call the new predicates. - lib/datadog/di/probe_notification_builder.rb (112): use `evaluate_at_entry?`. - sig/datadog/di/probe.rbs: signatures for the three predicates. - spec/datadog/di/probe_spec.rb: coverage for the three predicates. Behavior-preserving: the predicate bodies are the identical boolean expressions, and CaptureLimits.resolve(expr_limits: nil, ...) returns the identical limits hash. Verified locally: probe_spec (44), probe_notification_builder_spec (30), instrumenter_spec (112) all pass, 0 failures.
Address review comment (use `any` instead of `untyped`): switch the untyped values introduced by this PR in the DI signature subtree to the repo's `any` alias (`type any = untyped`, sig/datadog.rbs), documenting "accepts any type". Scoped to the signatures this PR added: - sig/datadog/di/context.rbs: entry_capture_expressions (ivar, ctor param, reader) - sig/datadog/di/serializer.rbs: serialize_args, serialize_vars, serialize_value - sig/datadog/di/probe_builder.rbs: build_from_remote_config, build_capture_expressions, build_capture_limits - sig/datadog/di/capture_expression_evaluator.rbs: evaluate `any` is identical to `untyped` (alias), so no type-checking behavior changes. Remaining pre-existing DI/symdb untyped are converted in a follow-up PR.
What does this PR do?
Adds capture-expression support to Ruby log probes in Dynamic Instrumentation. A capture expression is a named DSL expression attached to a log probe; at probe-fire time, the expression is evaluated against the probe scope and its serialized value is emitted in the snapshot payload under
captures.….captureExpressions.<name>. Capture expressions are an alternative tocaptureSnapshot: true: they let the user select exactly which values to capture instead of the whole local/argument scope.Motivation:
Feature parity with other languages.
Limitations:
Method probes (still) do not capture locals, therefore it is not possible to capture method locals via capture expressions also.
Positional arguments are (still) reported as
arg1etc. to the backend and users must specify that to capture them. Works but unintuitive.Change log entry
Yes. Dynamic Instrumentation log probes now support capture expressions as an alternative to capturing the full local/argument scope.
Manual Verification
UI:

Line probe:

Method probe - positional arg:

Method probe - keyword arg:
